Skip to content

order support #1813

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

order support #1813

wants to merge 2 commits into from

Conversation

blafond
Copy link
Member

@blafond blafond commented Dec 8, 2023

fixes #1735

  • implements and tests reactive session's setOrder(...)

@blafond blafond requested a review from DavideD December 8, 2023 20:10
@blafond blafond force-pushed the 1735_order_support branch from 9d1d8de to 3a00e10 Compare December 8, 2023 20:56
DavideD
DavideD requested changes Dec 11, 2023
View reviewed changes
Copy link
Member

@DavideD DavideD left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine overall.

Maybe I've missed it, but I don't think there's a test for setOrder(List<Order>)

@blafond
Copy link
Member Author

blafond commented Dec 12, 2023

I'll have an update in the morning to the test. Took your suggestions and it's looking cleaner and added a couple of tests, including the List<> case.

@DavideD
Copy link
Member

DavideD commented Dec 13, 2023

Looks good.
@blafond, could you check if these changes make sense to you?

@DavideD DavideD mentioned this pull request Dec 13, 2023
Merged
@DavideD
Copy link
Member

DavideD commented Dec 13, 2023

Superseded by #1815

@DavideD DavideD closed this Dec 13, 2023
@blafond
Copy link
Member Author

blafond commented Dec 13, 2023

Yep... I could have taking one more crack at simplifying the logic and reformatting. thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DavideD DavideD DavideD requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for Order
2 participants
@blafond @DavideD